refactor(*): remove workarounds for IE <9, log all parameters in IE 9 #15911
Conversation
mgol
force-pushed
the
ie
branch
3 times, most recently
from
8ec4ae5 to
ce05599
Compare
|
I think this should be split into separate commits:
I think it's acceptable that we only support Edge 15 for classes, especially since current FF is still choking on them, too. |
|
I can separate IE 9 log changes & Edge class fallback removal to teo
separate small commits. The other part is not just about removing IE 8-
stuff but also updating support comments related to IE/Edge. Would you like
to separate that as well? Those two have some overlap.
--
Michał Gołębiowski
|
|
3 commits sounds good - as long as the fixes have their own separate commits |
test/ng/logSpec.js
Outdated
| }) | ||
| ); | ||
| // Support: Safari 9.1 only, iOS 9.3 only | ||
| // For some reason Safar thinks there is always 1 parameter passed here. |
test/ng/logSpec.js
Outdated
| // For some reason Safar thinks there is always 1 parameter passed here. | ||
| if (!/\b9\.\d(\.\d+)* safari/i.test(window.navigator.userAgent) && | ||
| !/\biphone os 9_/i.test(window.navigator.userAgent)) { | ||
| it('should not attempt to log the second argument in IE if it is not specified', inject( |
There was a problem hiding this comment.
This test is a little weird. It's supposed to simulate the IE logging behavior (i.e. missing apply on console methods). Normally we'd just run those tests in IE without patching them but to be able to mock those methods we have to replace them with mocks that are regular functions so, in turn, we have to delete the methods.
Is there any way to reduce the confusion here?
There was a problem hiding this comment.
After taking a look at the whole describe block, I think it's fine.
I think we could now have a test that all arguments are logged (not just the first two as used to happen).
There was a problem hiding this comment.
@gkalpak I think we may even want to wrap the whole file in this block removing apply to simulate IE to have two test blocks - one with apply and another one without it; just to make sure there is now no observable difference in logging behavior with regular parameters in IE & non-IE.
I could do this cleanup in a separate PR, I don't want to drag this one further.
| error = function(arg1, arg2) { logger += 'error,' + arguments.length + ';'; }; | ||
| debug = function(arg1, arg2) { logger += 'debug,' + arguments.length + ';'; }; | ||
| }, | ||
| removeApplyFunctionForIE, |
There was a problem hiding this comment.
Nothing. I misinterpreted the change.
Please ignore 😁
IE 9 lacks apply on console methods but it's possible to borrow the apply method from Function.prototype.
|
PR updated & split into 3 commits. |
test/ng/logSpec.js
Outdated
| // For some reason Safar thinks there is always 1 parameter passed here. | ||
| if (!/\b9\.\d(\.\d+)* safari/i.test(window.navigator.userAgent) && | ||
| !/\biphone os 9_/i.test(window.navigator.userAgent)) { | ||
| it('should not attempt to log the second argument in IE if it is not specified', inject( |
There was a problem hiding this comment.
After taking a look at the whole describe block, I think it's fine.
I think we could now have a test that all arguments are logged (not just the first two as used to happen).
I tested them all manually in various IE/Edge versions. |
ES6 classes now require Edge 14 or newer to work. Closes angular#15911
In IE 9 console methods don't inherit from Function.prototype and, hence, don't have apply. Until recently IE 9 logging in AngularJS was restricted to the first 2 parameters but that changed as we could just reuse Function.prototype.apply everywhere, creating one code path for all browsers. Therefore, we can now run all tests in modes where apply exists on logging methods and where it doesn't. Ref angular#15911 Ref b277e3e
In IE 9 console methods don't inherit from Function.prototype and, hence, don't have apply. Until recently IE 9 logging in AngularJS was restricted to the first 2 parameters but that changed as we could just reuse Function.prototype.apply everywhere, creating one code path for all browsers. Therefore, we can now run all tests in modes where apply exists on logging methods and where it doesn't. Ref angular#15911 Ref b277e3e
In IE 9 console methods don't inherit from Function.prototype and, hence, don't have apply. Until recently IE 9 logging in AngularJS was restricted to the first 2 parameters but that changed as we could just reuse Function.prototype.apply everywhere, creating one code path for all browsers. Therefore, we can now run all tests in modes where apply exists on logging methods and where it doesn't. Ref #15911 Ref b277e3e Closes #15995
In IE 9 console methods don't inherit from Function.prototype and, hence, don't have apply. Until recently IE 9 logging in AngularJS was restricted to the first 2 parameters but that changed as we could just reuse Function.prototype.apply everywhere, creating one code path for all browsers. Therefore, we can now run all tests in modes where apply exists on logging methods and where it doesn't. Ref #15911 Ref b277e3e Closes #15995
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
A refactor.
What is the current behavior? (You can also link to an open issue here)
Some code targeting IE <=8 still exists in the code base, support comments don't account for Edge 15.
What is the new behavior (if this is a feature change)?
The opposite. Also,
$logis now able to log more than 2 parameters on IE and all browsers use one code path for passing parameters to a proper logging function.Does this PR introduce a breaking change?
No, unless we consider breaking support for classes in Edge 13 (current version is 15) a breaking change.
Please check if the PR fulfills these requirements
Other information: